Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use prototype based Recorder for performance boost. #12

Merged
merged 4 commits into from
Jan 26, 2016

Conversation

jamestalmage
Copy link
Contributor

The current method of creating a PowerAssertRecorder prevents some optimizations by the V8 compiler.

Specifically, it creates many extra closure contexts, and prevents the use of hidden classes.

My test suite was seeing roughly a 5% boost.

value: value,
events: this.captured
},
source: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this just be source: args? I did it this way to mimic what you had before, but I am not entirely sure why it's necessary to create a safe copy of args here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's redundant. source: args would be better. Renaming args would be more better :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Working on this now. It takes a while to update all the "expected" fixtures

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

@jamestalmage
Copy link
Contributor Author

@twada -

The function.toString() seems it to make it easier to edit the helper template. The only issue is it prevents coverage using nyc because of this. It really only seems to be an issue while I was trying to npm link this into ava (the solution was explicitly excluding **/babel-plugin-espower/** in my nyc config).

An alternate solution (should you ever want to add code coverage to this project), would be to move the helper to it's own file and just use fs.readFileSync.

@jamestalmage jamestalmage changed the title Use prototype based Recorder for perfromance boost. Use prototype based Recorder for performance boost. Jan 26, 2016
@twada
Copy link
Member

twada commented Jan 26, 2016

@jamestalmage Nice optimization!
Just out of curiosity, is function.toString() portable enough?

fs.readFileSync approach looks better to me.

@jamestalmage
Copy link
Contributor Author

fs.readFileSync added 😄

@jamestalmage
Copy link
Contributor Author

@twada - should be good to go

@twada
Copy link
Member

twada commented Jan 26, 2016

@jamestalmage Looks good to me. I'm going to merge and cut a new release.

twada added a commit that referenced this pull request Jan 26, 2016
Use prototype based Recorder for performance boost.
@twada twada merged commit 8d444b9 into power-assert-js:master Jan 26, 2016
@twada
Copy link
Member

twada commented Jan 26, 2016

@jamestalmage babel-plugin-espower 2.1.1 is out. Thanks!

@jamestalmage jamestalmage deleted the prototype-based-helper branch January 26, 2016 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants